-
Notifications
You must be signed in to change notification settings - Fork 16.5k
feat(mcp): add embeddable charts with guest token authentication #36933
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(mcp): add embeddable charts with guest token authentication #36933
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
CodeAnt AI finished reviewing your PR. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #36933 +/- ##
===========================================
+ Coverage 0 67.66% +67.66%
===========================================
Files 0 650 +650
Lines 0 48442 +48442
Branches 0 5284 +5284
===========================================
+ Hits 0 32777 +32777
- Misses 0 14377 +14377
- Partials 0 1288 +1288
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #e877b0
Actionable Suggestions - 1
-
superset-frontend/webpack.config.js - 1
- Path format inconsistency · Line 300-304
Additional Suggestions - 8
-
superset/embedded_chart/api.py - 3
-
API Response Consistency · Line 110-111Ensure datasource_id and datasource_type are not null in the JSON response to match the OpenAPI schema, using defaults consistent with GetExplorePermalinkCommand logic.
-
Type Safety Improvement · Line 99-104Add type hints to local variables to comply with type safety standards for new Python code. This improves maintainability and aligns with MyPy requirements.
-
Logging Cleanup · Line 118-118Remove redundant %s ex from logger.exception, as the method already logs the full traceback and exception details.
-
-
superset/embedded_chart/exceptions.py - 4
-
Messages not internationalized · Line 21-21Exception messages appear to be user-facing and should be internationalized using flask_babel._() for consistency with other Superset exceptions like SupersetMarshmallowValidationError. This ensures messages can be translated if the application supports multiple languages.
-
Missing docstring · Line 20-20This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when an embedded chart permalink is not found or has expired.'
Code suggestion
@@ -20,2 +20,3 @@ - class EmbeddedChartPermalinkNotFoundError(SupersetException): - message = "The embedded chart permalink could not be found or has expired." + class EmbeddedChartPermalinkNotFoundError(SupersetException): + """Raised when an embedded chart permalink is not found or has expired.""" + message = "The embedded chart permalink could not be found or has expired."
-
Missing docstring · Line 24-24This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when access to an embedded chart is denied.'
Code suggestion
@@ -24,2 +24,4 @@ - class EmbeddedChartAccessDeniedError(SupersetException): - message = "Access to this embedded chart is denied." + class EmbeddedChartAccessDeniedError(SupersetException): + """Raised when access to an embedded chart is denied.""" + message = "Access to this embedded chart is denied."
-
Missing docstring · Line 28-28This new exception class lacks a docstring, which is required for all new classes per the project's coding standards. Consider adding a brief description like 'Raised when the embedded chart feature is disabled.'
-
-
superset-frontend/src/embeddedChart/index.tsx - 1
-
Inline styles usage · Line 80-80The code uses inline styles in several places (e.g., error messages, loading containers, chart wrapper), which may conflict with the guideline to 'avoid custom css and styles' and follow antd best practices whenever possible. While necessary for embedded viewport filling, consider refactoring to use antd's Flex or Space components for layout, or extract to a CSS module if custom styling is unavoidable.
-
Review Details
-
Files reviewed - 15 · Commit Range:
99b1a97..99b1a97- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/config.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
Code Review Responses✅ Fixed Issues
❌ Not Applicable / Incorrect Suggestions
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #a3d995
Actionable Suggestions - 1
-
superset/security/manager.py - 1
- Blind exception catch too broad · Line 2850-2851
Additional Suggestions - 2
-
superset-frontend/src/embeddedChart/index.tsx - 2
-
Improved null safety · Line 125-126The added null check for event.data prevents potential TypeError if data is null/undefined, aligning with TypeScript best practices for MessageEvent handling.
-
Enhanced error handling · Line 292-298Wrapping start() in .catch() handles async failures gracefully, logging errors and resetting state, which improves reliability for embedded chart initialization.
-
Review Details
-
Files reviewed - 16 · Commit Range:
99b1a97..4be0466- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/config.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
Code Review Agent Run #86accdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Code Review Agent Run #0b3e6dActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #42298fActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #b9e5b1
Actionable Suggestions - 1
-
superset/embedded_chart/view.py - 1
- Security Bug in Referrer Validation · Line 42-53
Review Details
-
Files reviewed - 18 · Commit Range:
2b4b4f8..1e6c4ee- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/webpack.config.js
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/security/guest_token.py
- superset/security/manager.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
| def same_origin(url1: str | None, url2: str | None) -> bool: | ||
| """Check if two URLs have the same origin (scheme + netloc).""" | ||
| if not url1 or not url2: | ||
| return False | ||
| parsed1 = urlparse(url1) | ||
| parsed2 = urlparse(url2) | ||
| # For domain matching, we just check if the host matches | ||
| # url2 might just be a domain like "example.com" | ||
| if not parsed2.scheme: | ||
| # url2 is just a domain, check if it matches url1's netloc | ||
| return parsed1.netloc == url2 or parsed1.netloc.endswith(f".{url2}") | ||
| return (parsed1.scheme, parsed1.netloc) == (parsed2.scheme, parsed2.netloc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same_origin function incorrectly uses netloc (which includes ports) for domain matching, causing it to reject valid referrers with non-standard ports like 'https://example.com:8080' when 'example.com' is allowed. It should use hostname for host-only comparison and perform case-insensitive matching, as hosts are case-insensitive per RFC.
Code Review Run #b9e5b1
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #4834de
Actionable Suggestions - 3
-
superset/config.py - 1
- Breaking change in stable feature flag · Line 594-595
-
superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx - 1
- Error Handling Issue · Line 142-146
-
superset/charts/api.py - 1
- Missing Permission Decorator · Line 1247-1339
Additional Suggestions - 9
-
tests/integration_tests/charts/api_tests.py - 1
-
Unrestricted embedded permissions · Line 309-312These embedded permissions are added to the expected set, but unlike "can_set_embedded", they are not restricted in the security manager. This could allow unauthorized access to embedded configurations. Consider adding them to ADMIN_ONLY_PERMISSIONS to match the pattern for admin-only features.
-
-
superset/charts/api.py - 1
-
Incomplete Transaction Handling · Line 1320-1339The try block only catches ValidationError from schema load, but if EmbeddedChartDAO.upsert raises an exception, the session won't rollback, risking partial DB commits.
-
-
superset/daos/chart.py - 1
-
Type Hint Error · Line 128-128The type hint for 'item' in the create method appears incorrect, as it references the DAO class rather than the model type expected by BaseDAO[EmbeddedChart]. This could cause MyPy type checking failures, though it won't affect runtime since the method raises NotImplementedError.
Code suggestion
@@ -128,1 +128,1 @@ -item: EmbeddedChartDAO | None = None, +item: EmbeddedChart | None = None,
-
-
superset/models/embedded_chart.py - 1
-
Property logic refinement · Line 53-58The `allowed_domains` property could include empty strings if `allow_domain_list` has consecutive commas or leading/trailing commas, which might cause unexpected behavior in embedding checks. Consider refining the logic to handle malformed input gracefully.
Code suggestion
@@ -53,6 +53,6 @@ @property def allowed_domains(self) -> list[str]: """ A list of domains which are allowed to embed the chart. An empty list means any domain can embed. """ - return self.allow_domain_list.split(",") if self.allow_domain_list else [] + return [domain.strip() for domain in self.allow_domain_list.split(",") if domain.strip()] if self.allow_domain_list else []
-
-
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx - 1
-
Icon choice for embed menu item · Line 501-501The icon for the 'Embed chart' menu item was changed to ExportOutlined, but embedding charts typically involves code generation, so CodeOutlined is more appropriate.
Code suggestion
@@ -501,1 +501,1 @@ - icon: <Icons.ExportOutlined css={dropdownIconsStyles} />, + icon: <Icons.CodeOutlined css={dropdownIconsStyles} />,
-
-
superset/views/embedded_charts.py - 1
-
Missing Documentation · Line 33-33The list method is missing a docstring. Per the project's coding standards in AGENTS.md and .cursor/rules/dev-standard.mdc, docstrings are required for new functions and methods to improve maintainability.
Code suggestion
@@ -31,6 +31,7 @@ @expose("/list/") @has_access def list(self) -> FlaskResponse: + """Render the embedded charts list page.""" return super().render_app_template()
-
-
superset/initialization/__init__.py - 1
-
Category Mismatch · Line 526-533The category "Security" may not fit for EmbeddedChartsView, as it's chart management rather than security. Other chart views like SliceModelView use no category. If embedding has security implications, keep as is.
-
-
superset/commands/chart/delete.py - 1
-
Code Structure Inconsistency · Line 81-81The run method here returns the result of EmbeddedChartDAO.delete, but the similar DeleteChartCommand.run method in the same file does not return. For consistency within the codebase, consider removing the return statement.
-
-
superset/charts/schemas.py - 1
-
Schema metadata suggestion · Line 348-356The new EmbeddedChartConfigSchema and EmbeddedChartResponseSchema classes are correctly defined with appropriate field types and requirements that match their usage in the charts API. However, consider adding metadata to the fields for better OpenAPI documentation, as seen in other schemas in this file.
-
Review Details
-
Files reviewed - 37 · Commit Range:
1e6c4ee..66ad889- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/SliceHeaderControls.test.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/components/menu/ShareMenuItems/ShareMenuItems.test.tsx
- superset-frontend/src/dashboard/components/menu/ShareMenuItems/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
- superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
- superset-frontend/src/pages/EmbeddedChartsList/index.tsx
- superset-frontend/src/views/routes.tsx
- superset-frontend/webpack.config.js
- superset/charts/api.py
- superset/charts/schemas.py
- superset/commands/chart/delete.py
- superset/commands/chart/exceptions.py
- superset/config.py
- superset/daos/chart.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/migrations/versions/2026-01-08_16-45_50aff65919a0_add_embedded_charts_table.py
- superset/models/embedded_chart.py
- superset/models/slice.py
- superset/security/guest_token.py
- superset/security/manager.py
- superset/views/embedded_charts.py
- tests/integration_tests/charts/api_tests.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
| # Enable caching per user key for Superset cache (not database cache impersonation) | ||
| "CACHE_QUERY_BY_USER": False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the stable EMBEDDABLE_CHARTS feature flag from DEFAULT_FEATURE_FLAGS disables chart embedding by default, which is a breaking change for users relying on the default configuration. According to FEATURE_FLAGS.md and official docs, this flag is still stable and not deprecated. If this removal is intentional, consider adding a deprecation notice first.
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| .then(({ result }) => { | ||
| setReady(true); | ||
| setEmbedded(result); | ||
| setAllowedDomains(result ? result.allowed_domains.join(', ') : ''); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The useEffect fails to set ready to true on API errors, potentially leaving the component stuck in loading state. Add a .finally block to ensure ready is always set.
Code suggestion
Check the AI-generated fix before applying
@@ -141,6 +141,7 @@
.then(({ result }) => {
setReady(true);
setEmbedded(result);
setAllowedDomains(result ? result.allowed_domains.join(', ') : '');
})
+ .finally(() => setReady(true));
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| @expose("/<id_or_uuid>/embedded", methods=["POST", "PUT"]) | ||
| @protect() | ||
| @safe | ||
| @statsd_metrics | ||
| @event_logger.log_this_with_context( | ||
| action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.set_embedded", | ||
| log_to_statsd=False, | ||
| ) | ||
| def set_embedded(self, id_or_uuid: str) -> Response: | ||
| """Set a chart's embedded configuration. | ||
| --- | ||
| post: | ||
| summary: Set a chart's embedded configuration | ||
| parameters: | ||
| - in: path | ||
| schema: | ||
| type: string | ||
| name: id_or_uuid | ||
| description: The chart id or uuid | ||
| requestBody: | ||
| description: The embedded configuration to set | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: EmbeddedChartConfigSchema | ||
| responses: | ||
| 200: | ||
| description: Successfully set the configuration | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| result: | ||
| $ref: '#/components/schemas/EmbeddedChartResponseSchema' | ||
| 401: | ||
| $ref: '#/components/responses/401' | ||
| 404: | ||
| $ref: '#/components/responses/404' | ||
| 500: | ||
| $ref: '#/components/responses/500' | ||
| put: | ||
| description: >- | ||
| Sets a chart's embedded configuration. | ||
| parameters: | ||
| - in: path | ||
| schema: | ||
| type: string | ||
| name: id_or_uuid | ||
| description: The chart id or uuid | ||
| requestBody: | ||
| description: The embedded configuration to set | ||
| required: true | ||
| content: | ||
| application/json: | ||
| schema: EmbeddedChartConfigSchema | ||
| responses: | ||
| 200: | ||
| description: Successfully set the configuration | ||
| content: | ||
| application/json: | ||
| schema: | ||
| type: object | ||
| properties: | ||
| result: | ||
| $ref: '#/components/schemas/EmbeddedChartResponseSchema' | ||
| 401: | ||
| $ref: '#/components/responses/401' | ||
| 404: | ||
| $ref: '#/components/responses/404' | ||
| 500: | ||
| $ref: '#/components/responses/500' | ||
| """ | ||
| try: | ||
| chart = ChartDAO.get_by_id_or_uuid(id_or_uuid) | ||
| except ChartNotFoundError: | ||
| return self.response_404() | ||
|
|
||
| try: | ||
| body = self.embedded_config_schema.load(request.json) | ||
|
|
||
| embedded = EmbeddedChartDAO.upsert( | ||
| chart, | ||
| body["allowed_domains"], | ||
| ) | ||
| db.session.commit() # pylint: disable=consider-using-transaction | ||
|
|
||
| result = self.embedded_response_schema.dump(embedded) | ||
| return self.response(200, result=result) | ||
| except ValidationError as error: | ||
| db.session.rollback() # pylint: disable=consider-using-transaction | ||
| return self.response_400(message=error.messages) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The set_embedded endpoint lacks a @permission_name decorator, unlike get_embedded ('read') and delete_embedded ('set_embedded'). This could allow unauthorized config changes if permissions aren't enforced elsewhere.
Citations
- Rule Violated: AGENTS.md:85
Code Review Run #4834de
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #d77337Actionable Suggestions - 0Additional Suggestions - 5
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Code Review Agent Run #86e5b3Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Add a new MCP tool `get_embeddable_chart` that enables AI agents to create ephemeral chart visualizations with guest token authentication. This allows charts to be embedded in external applications (Claude Desktop, ChatGPT, etc.) without persisting them to the database. Key changes: - Add CHART_PERMALINK resource type to GuestTokenResourceType enum - Add validation for chart_permalink resources in SecurityManager - Add EMBEDDABLE_CHARTS_MCP feature flag (disabled by default) - Create embedded_chart backend module with API and view - Create MCP tool with schemas for get_embeddable_chart - Create frontend embeddedChart entry using StatefulChart component The feature uses the existing permalink system for chart state storage with TTL support, and leverages StatefulChart for lightweight rendering that handles data fetching internally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: address CI failures for embeddable charts PR - Remove literal color apache#666 from embeddedChart (use default text color) - Shorten long description lines in schemas.py - Fix import ordering in app.py - Add EmbeddedChartRestApi and EmbeddedChartView to security allowlist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: address code review feedback for embeddable charts - Fix null pointer in validateMessageEvent (typeof null === 'object') - Fix race condition with started flag - reset on failure - Fix security issue - don't leak exception message in 500 response - Add docstrings to exception classes - Remove redundant %s formatting in logger.exception 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: remove exception chaining to prevent internal detail leakage Remove `from ex` in exception handling to prevent leaking internal exception details. Re-raise EmbeddedChartPermalinkNotFoundError directly if already raised, otherwise raise a new instance without chaining. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: use 'from None' to satisfy ruff B904 rule Within an except clause, using 'from None' explicitly suppresses exception chaining which prevents internal error details from being exposed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: format with ruff 0.9.7 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: format embeddedChart with prettier 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: add proper type annotations for mypy compliance - Import GuestTokenUser, GuestTokenResource, GuestTokenRlsRule types - Add type annotations to guest_user, resources, rls_rules - Use GuestTokenResourceType enum directly instead of .value - Handle bytes return type from create_guest_access_token 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix: use .get() for GUEST_TOKEN_HEADER_NAME config access Provides a default value to prevent potential KeyError. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: re-trigger CI for flaky test The sharded-jest-tests (3) failure is a flaky test timeout (DatasourceEditor.test.tsx) unrelated to this PR's changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add referrer validation and @Protect() decorator for embeddable charts Security improvements based on code review comparing with dashboard embedding: 1. Add optional allowed_domains parameter to get_embeddable_chart MCP tool - Stored in permalink state for later validation - Allows restricting which domains can embed the chart 2. Add referrer validation to EmbeddedChartView - Validates request.referrer against allowed_domains - Uses same same_origin() check as dashboard embedding - Returns 403 if referrer not in allowed domains list 3. Add @Protect() decorator to EmbeddedChartRestApi.get() - Consistent with EmbeddedDashboardRestApi pattern - Adds class_permission_name for FAB permissions 4. Include allowed_domains in API response - Useful for debugging and validation 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> chore: re-trigger CI for flaky test The sharded-jest-tests (3) failure is a flaky test timeout (DatasourceEditor.test.tsx) unrelated to this PR's changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): fix API response format and PyJWT compatibility for embeddable charts 1. Fix API response format to match frontend expectations: - Frontend expects { state: { formData: {...} } } - Was returning { result: { form_data: {...} } } - Now returns state directly from permalink 2. Fix PyJWT compatibility: - PyJWT 2.0+ returns string, older versions return bytes - Handle both cases when decoding guest token 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add guest token security for embedded chart data access - Add guest token validation in embedded_chart API to verify permalink access - Add has_guest_chart_permalink_access() to security manager to allow guest users with chart_permalink resources to access datasources - Add can_read CurrentUserRestApi to PUBLIC_ROLE_PERMISSIONS for frontend initialization in embedded views - Remove redundant referrer validation from view (now handled by guest token) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): remove EMBEDDABLE_CHARTS_MCP feature flag, use EMBEDDED_SUPERSET Per PR review feedback, avoid adding a new feature flag since MCP is already feature-flagged. Embedded charts now use the existing EMBEDDED_SUPERSET flag which is consistent with dashboard embedding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): remove all feature flag checks from embedded charts Per PR review feedback, remove all feature flag usage. The embedded chart functionality is now always available - no feature flags needed. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): add isinstance check for GuestUser in has_guest_chart_permalink_access The method was failing for regular User objects that don't have a resources attribute. Added explicit isinstance(user, GuestUser) check to ensure we only access user.resources for actual GuestUser instances. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]> fix(mcp): fix ruff linting issues in embedded chart files - Fix import sorting in api.py - Remove unused imports in view.py (Any, cast, same_origin, GetExplorePermalinkCommand) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add "Embed chart" option to chart header dropdown menu on dashboards. When clicked, opens a modal that generates embed code (iframe + guest token) using the existing permalink + guest token infrastructure. Changes: - Add POST /api/v1/embedded_chart/ endpoint to create embeddable charts - Add EmbeddedChartModal component with allowed domains and TTL settings - Add "Embed chart" menu item to SliceHeaderControls - Add EmbedChart to MenuKeys enum This brings UI parity with PR apache#33424's approach while using our simpler ephemeral permalink-based architecture (no database model needed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add CORS/referrer validation to embedded chart view, matching the dashboard embedding behavior. If allowed_domains is configured in the permalink state, validate that the request referrer matches one of the allowed domains before rendering. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Remove CodeBlock styled component using deprecated theme.colors - Change CodeOutlined icon to ExportOutlined (valid icon) - Add chartId prop to EmbeddedChartModal - Simplify EmbeddedChartModal to use existing embedded chart API - Fix ESLint issues (duplicate imports, destructuring, useEffect pattern) - Replace console.error with logging.error 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Add new embedded chart permissions to the expected permissions set in test_info_security_chart test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
The Embed code option (simple iframe URL) was previously always available with EMBEDDABLE_CHARTS: True by default. Keep it always visible since it's a simple feature. Only gate the new Embed chart option (which uses guest tokens) behind the EMBEDDED_SUPERSET feature flag. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Revert to using EMBEDDABLE_CHARTS instead of EMBEDDED_SUPERSET for chart embedding features to maintain backwards compatibility. - EMBEDDABLE_CHARTS: True by default (for Embed code/chart options) - EMBEDDED_SUPERSET: False by default (for dashboard embedding with guest tokens) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add @permission_name("set_embedded") decorator to set_embedded endpoint
- Fix error handling in EmbeddedChartModal to always set ready state
- Add datasource validation in embedded chart API create endpoint
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- EmbedCodeContent now calls /api/v1/embedded_chart/ to generate guest token and proper iframe with postMessage script - embeddedChart/index.tsx now accepts simple direct guestToken messages without requiring Switchboard protocol 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
8128449 to
8c16c22
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Simple HTML page to test embedding charts with guest token authentication. Paste the iframe_html from get_embeddable_chart MCP tool response to test. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
|
|
||
| export const ChartEmbedControls = ({ chartId, onHide }: Props) => { | ||
| const { addInfoToast, addDangerToast } = useToasts(); | ||
| const [ready, setReady] = useState(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The component initializes the ready state to true, so the UI renders as if data is loaded before the GET request completes; this causes a flash of the main UI and bypasses the intended loading indicator. Initialize ready to false so the Loading spinner is shown until the fetch finishes. [logic error]
Severity Level: Critical 🚨
- ❌ Embed Chart modal flashes uninitialized content.
- ⚠️ Visual flicker when opening Embed Chart modal.| const [ready, setReady] = useState(true); | |
| const [ready, setReady] = useState(false); |
Steps of Reproduction ✅
1. Open the Dashboard UI and click the chart "Embed chart" action that mounts the
EmbeddedChartModal component (UI flow described in PR). The modal renders the
ChartEmbedControls component defined at
`superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx:61`.
2. On the initial render of ChartEmbedControls (component start at line 61), the code at
`index.tsx:63` initializes ready with `useState(true)`. Because ready is true, the
component proceeds to render the main UI instead of the Loading indicator.
3. Immediately after mount the useEffect at `index.tsx:129` runs and calls
`setReady(false)` to start the GET request for `/api/v1/chart/${chartId}/embedded` (see
lines 129-134). This causes a visible flash: the main UI is shown briefly, then the
component re-renders into the Loading state (`if (!ready) { return <Loading />; }` at
lines 151-153).
4. Expected behavior: Loading should show until the GET completes. The root cause is the
initial value at `index.tsx:63`. Initializing ready to false prevents the UI flash. If you
believe this pattern is intentional, note that the effect already sets ready false on
mount, but that still allows an initial render with ready=true and causes the flash.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
**Line:** 63:63
**Comment:**
*Logic Error: The component initializes the `ready` state to true, so the UI renders as if data is loaded before the GET request completes; this causes a flash of the main UI and bypasses the intended loading indicator. Initialize `ready` to false so the Loading spinner is shown until the fetch finishes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| show={embedModalIsOpen} | ||
| onHide={() => setEmbedModalIsOpen(false)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Prop-name mismatch on the rendered modal: other modals in this file use showModal / onHideModal naming (e.g. DrillDetailModal), but the new EmbeddedChartModal is rendered with show and onHide. If EmbeddedChartModal follows the same API as other dashboard modals, passing show/onHide will do nothing and the modal won't open/close as expected. Change the prop names to match the modal API used elsewhere (use showModal and onHideModal) so the component receives the visible state and close callback. [possible bug]
Severity Level: Critical 🚨
- ❌ Dashboard "Embed chart" modal fails to open.
- ⚠️ Dashboard embed UI unusable for sharable charts.| show={embedModalIsOpen} | |
| onHide={() => setEmbedModalIsOpen(false)} | |
| showModal={embedModalIsOpen} | |
| onHideModal={() => setEmbedModalIsOpen(false)} |
Steps of Reproduction ✅
1. Open a dashboard and open a chart's dropdown. The dropdown Menu is rendered with
onClick={handleMenuClick} in this file (SliceHeaderControls). The handleMenuClick switch
includes the EmbedChart branch at src/dashboard/components/SliceHeaderControls/index.tsx
lines ~292-299 where it calls setEmbedModalIsOpen(true).
2. Select the "Embed chart" menu entry (MenuKeys.EmbedChart). The case MenuKeys.EmbedChart
in handleMenuClick at index.tsx: ~294-298 executes setEmbedModalIsOpen(true).
3. The component tree includes <EmbeddedChartModal ... /> rendered at index.tsx:621-626.
That component instance receives props show and onHide (lines 623-625), not the
showModal/onHideModal shape used by other modals in this file (for example
DrillDetailModal in the same component uses showModal and onHideModal).
4. Because EmbeddedChartModal is passed show/onHide instead of showModal/onHideModal, the
modal's visibility API is not driven by the local state. Result: clicking "Embed chart"
sets embedModalIsOpen to true (verified at handleMenuClick: index.tsx:~294-298) but the
EmbeddedChartModal does not open. This reproduces the broken UI where the embed modal
never appears when the menu item is used.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
**Line:** 624:625
**Comment:**
*Possible Bug: Prop-name mismatch on the rendered modal: other modals in this file use `showModal` / `onHideModal` naming (e.g. `DrillDetailModal`), but the new `EmbeddedChartModal` is rendered with `show` and `onHide`. If `EmbeddedChartModal` follows the same API as other dashboard modals, passing `show`/`onHide` will do nothing and the modal won't open/close as expected. Change the prop names to match the modal API used elsewhere (use `showModal` and `onHideModal`) so the component receives the visible state and close callback.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
|
||
| function validateMessageEvent(event: MessageEvent) { | ||
| const { data } = event; | ||
| if (data == null || typeof data !== 'object' || data.type !== MESSAGE_TYPE) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Logic/compatibility bug: validateMessageEvent currently rejects any message that does not have data.type === MESSAGE_TYPE, which will also reject legitimate direct guest-token or handshake messages that omit type. As a result, valid messages carrying guestToken or handshake may be ignored. Relax validation to allow messages that carry guestToken or handshake in addition to the typed messages. [logic error]
Severity Level: Critical 🚨
- ❌ Embed auth initialization fails; guest token ignored.
- ❌ Embedded charts fail to load without handshake.| if (data == null || typeof data !== 'object' || data.type !== MESSAGE_TYPE) { | |
| // Accept messages that explicitly use our MESSAGE_TYPE, or messages that | |
| // carry a guestToken or handshake for backward compatibility. | |
| if ( | |
| data == null || | |
| typeof data !== 'object' || | |
| (data.type !== MESSAGE_TYPE && !('guestToken' in data) && !('handshake' in data)) | |
| ) { |
Steps of Reproduction ✅
1. Host an iframe pointing to the embeddable page
(superset-frontend/src/embeddedChart/index.tsx).
2. From the parent page, send a postMessage to the iframe using window.postMessage({
guestToken: 'abc' }, targetOrigin) without a `type` field.
3. The embedded page receives the message and the listener at line 287
(window.addEventListener) immediately calls validateMessageEvent at lines 129-134.
4. validateMessageEvent enforces `data.type === MESSAGE_TYPE` and throws because `type` is
absent; the listener's try/catch (lines 287-293) catches this and logs "ignoring message
unrelated to embedded comms" and returns, so the guestToken handling branches (lines
296-307 / Switchboard flow at 310+) never run.
5. Observed outcome: guest token is ignored and the embedded app does not initialize
authentication or render charts.
Note: This is a concrete failure when parent code posts guestToken without adding a `type`
property (a likely integration pattern).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/embeddedChart/index.tsx
**Line:** 131:131
**Comment:**
*Logic Error: Logic/compatibility bug: `validateMessageEvent` currently rejects any message that does not have `data.type === MESSAGE_TYPE`, which will also reject legitimate direct guest-token or handshake messages that omit `type`. As a result, valid messages carrying `guestToken` or `handshake` may be ignored. Relax validation to allow messages that carry `guestToken` or `handshake` in addition to the typed messages.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| } | ||
|
|
||
| resources: list[GuestTokenResource] = [ | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The resource type is passed as an enum object when creating the guest token resources, but later code (and token payloads) expect the string value; this can cause a mismatch during serialization or when validating the guest token (access will be denied because the types won't match). Change the resource type to use the enum's .value so a string is stored. [type error]
Severity Level: Critical 🚨
- ❌ POST /api/v1/embedded_chart/ creation fails.
- ❌ Generated guest token rejected by access checks.
- ⚠️ UI \"Embed chart\" flow returns errors.
- ⚠️ MCP tool embed creation may error.| { | |
| "type": GuestTokenResourceType.CHART_PERMALINK.value, |
Steps of Reproduction ✅
1. Call the embeddable-chart creation API: POST /api/v1/embedded_chart/ which maps to
EmbeddedChartRestApi.post in superset/embedded_chart/api.py (post method defined around
lines 177-301 in the PR).
2. Execution reaches the guest-token creation block at
superset/embedded_chart/api.py:lines 261-283 (guest_user and resources construction),
specifically the resources dict at lines 267-272 is built with
GuestTokenResourceType.CHART_PERMALINK (enum instance).
3. The code calls security_manager.create_guest_access_token(...) at
superset/embedded_chart/api.py:lines 276-281. The guest token creation serializes the
resources payload into a JWT JSON body.
4. Because the resource \"type\" contains an Enum object
(GuestTokenResourceType.CHART_PERMALINK) rather than a primitive string, JSON
serialization or downstream validation either (a) raises a serialization error or (b)
produces a payload that does not match the string-based checks in
_validate_guest_token_access (superset/embedded_chart/api.py:_validate_guest_token_access,
lines ~58-75). The POST request therefore either returns HTTP 500 (serialization error) or
issues a guest token that will fail access checks when used to fetch the embed (leading to
401).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/embedded_chart/api.py
**Line:** 269:269
**Comment:**
*Type Error: The resource `type` is passed as an enum object when creating the guest token resources, but later code (and token payloads) expect the string value; this can cause a mismatch during serialization or when validating the guest token (access will be denied because the types won't match). Change the resource `type` to use the enum's `.value` so a string is stored.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| # Build iframe URL using request host | ||
| base_url = request.host_url.rstrip("/") | ||
| iframe_url = f"{base_url}/embedded/chart/?permalink_key={permalink_key}" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The GET endpoint expects the permalink as a path parameter, but the generated iframe_url places the permalink as a query parameter; this mismatch means the iframe URL will not match the API route and the iframe will fail to load the embed. Construct the iframe URL using the path-style route (embed path + permalink) to match the GET route. [logic error]
Severity Level: Critical 🚨
- ❌ Embedded iframe fails to load (404).
- ⚠️ Dashboard UI \"Embed chart\" unusable.
- ⚠️ Third-party embeds get invalid URLs.| iframe_url = f"{base_url}/embedded/chart/{permalink_key}" |
Steps of Reproduction ✅
1. Create an embed by calling POST /api/v1/embedded_chart/ (EmbeddedChartRestApi.post in
superset/embedded_chart/api.py, method body lines ~177-301). The response includes the
iframe_url built at superset/embedded_chart/api.py:lines 291-292.
2. Take the returned iframe_url (example: https://host/embedded/chart/?permalink_key=XYZ)
and load it in a browser iframe.
3. Browser requests GET https://host/embedded/chart/ without a path parameter; the GET
handler is defined as @expose("/<permalink_key>", methods=("GET",)) in
superset/embedded_chart/api.py (get method at lines ~94-166). Because the path parameter
is missing, Flask routing does not match the GET endpoint for the permalink and the iframe
request yields 404/invalid route instead of returning the chart state.
4. Result: embedded iframe fails to load chart configuration; frontend cannot render
embed. The mismatch is reproducible using the API response from step 1 and loading that
iframe_url.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/embedded_chart/api.py
**Line:** 293:293
**Comment:**
*Logic Error: The GET endpoint expects the permalink as a path parameter, but the generated `iframe_url` places the permalink as a query parameter; this mismatch means the iframe URL will not match the API route and the iframe will fail to load the embed. Construct the iframe URL using the path-style route (embed path + permalink) to match the GET route.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| return EmbeddedChartDAO.delete(self._chart.embedded) | ||
|
|
||
| def validate(self) -> None: | ||
| pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The validate method is a no-op, allowing deletion to proceed without verifying the embedded chart exists, whether there are associated reports, or whether the caller has ownership; implement the same existence, report-association, and ownership checks used in DeleteChartCommand.validate to prevent unauthorized or invalid deletes. [security]
Severity Level: Critical 🚨
- ❌ Embedded deletion bypasses ownership checks.
- ❌ Deletion can proceed despite associated reports.
- ❌ TypeError when embedded is missing.
- ⚠️ Affects command layer: superset/commands/chart/delete.py.| pass | |
| # Ensure chart and embedded object exist | |
| if not self._chart or getattr(self._chart, "embedded", None) is None: | |
| raise ChartNotFoundError() | |
| # Prevent deletion if there are associated ReportSchedules | |
| if reports := ReportScheduleDAO.find_by_chart_ids([self._chart.id]): | |
| report_names = [report.name for report in reports] | |
| raise ChartDeleteFailedReportsExistError( | |
| _( | |
| "There are associated alerts or reports: %(report_names)s", | |
| report_names=",".join(report_names), | |
| ) | |
| ) | |
| # Check ownership | |
| try: | |
| security_manager.raise_for_ownership(self._chart) | |
| except SupersetSecurityException as ex: | |
| raise ChartForbiddenError() from ex |
Steps of Reproduction ✅
1. Call DeleteEmbeddedChartCommand.run():
- File: superset/commands/chart/delete.py
- run() is defined at lines 78-81 and calls self.validate() then proceeds.
- Current validate() at lines 83-84 is a no-op.
2. Create a chart that has no embedded object (embedded is None) and call run():
- Because validate() is a no-op, run() will call EmbeddedChartDAO.delete(None),
which then raises TypeError in superset/daos/base.py:423-440 when iterating None.
3. Create a chart that has associated reports and call run():
- The DAO ReportSchedule check exists in DeleteChartCommand.validate (lines 52-71),
but not here. ReportScheduleDAO.find_by_chart_ids is implemented at
superset/daos/report.py:54-59.
- Because validate() is empty, deletion will proceed even if reports exist, bypassing
the
safeguards that prevent deletion in DeleteChartCommand.
4. Call run() as a non-owner:
- DeleteChartCommand.validate enforces ownership via
security_manager.raise_for_ownership
(superset/commands/chart/delete.py lines 52-71). The empty validate() here omits that
check,
allowing potential unauthorized deletion if callers invoke this command.
Explanation: The no-op validate effectively omits existence, report-association, and
ownership checks
that are present for regular chart deletion and therefore is a real security/consistency
gap.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/commands/chart/delete.py
**Line:** 84:84
**Comment:**
*Security: The `validate` method is a no-op, allowing deletion to proceed without verifying the embedded chart exists, whether there are associated reports, or whether the caller has ownership; implement the same existence, report-association, and ownership checks used in `DeleteChartCommand.validate` to prevent unauthorized or invalid deletes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| "GLOBAL_ASYNC_QUERIES": False, | ||
| "EMBEDDED_SUPERSET": False, | ||
| # Enables the "Embed code" and "Embed chart" options in the Share menu | ||
| "EMBEDDABLE_CHARTS": True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Security risk: enabling embeddable charts by default exposes the feature while the guest token secret in this repository is the insecure default, which can allow unauthorized embedded access. Disable the feature by default so operators must explicitly opt-in after configuring a strong GUEST_TOKEN_JWT_SECRET. [security]
Severity Level: Critical 🚨
- ❌ Embeddable chart creation endpoint exposed by default.
- ❌ Guest tokens signed with insecure default secret.
- ⚠️ Many deployments use shipped defaults unchanged.| "EMBEDDABLE_CHARTS": True, | |
| "EMBEDDABLE_CHARTS": False, |
Steps of Reproduction ✅
1. Start Superset with the repository default configuration (no superset_config override).
The DEFAULT_FEATURE_FLAGS dict in superset/config.py includes the lines at
`superset/config.py:562-563` showing `"EMBEDDABLE_CHARTS": True`.
2. Verify embedded configuration values present in same file: the default guest secret
`GUEST_TOKEN_JWT_SECRET` is set to the insecure default (`"test-guest-secret-change-me"`)
in superset/config.py (Embedded config options section).
3. Use the UI flow described in the PR (Dashboard → Chart dropdown → "Embed chart" → POST
/api/v1/embedded_chart/) which will be enabled when EMBEDDABLE_CHARTS is True; the
endpoint will generate a guest token signed with the configured `GUEST_TOKEN_JWT_SECRET`.
4. Because EMBEDDABLE_CHARTS defaults to True and the guest secret is the insecure
default, a deployment that hasn't overridden the secret will issue guest tokens signed
with the known default, enabling unauthorized embeddable access. This reproduces the risk
without any codepath modifications.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/config.py
**Line:** 563:563
**Comment:**
*Security: Security risk: enabling embeddable charts by default exposes the feature while the guest token secret in this repository is the insecure default, which can allow unauthorized embedded access. Disable the feature by default so operators must explicitly opt-in after configuring a strong `GUEST_TOKEN_JWT_SECRET`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
CodeAnt AI Incremental review completed. |
- t now exported from @apache-superset/core/ui - logging now exported from @apache-superset/core 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review Agent Run #5db474
Actionable Suggestions - 2
-
superset/embedded_chart/api.py - 1
- Incorrect iframe URL format · Line 290-292
-
superset/charts/schemas.py - 1
- Schema field type inconsistency · Line 352-356
Additional Suggestions - 10
-
superset/security/manager.py - 1
-
Security: Overly permissive datasource access · Line 2971-3003The method currently grants datasource access if the user has any chart permalink, which could allow unauthorized access to datasources not intended for the user. While the embedded chart API validates specific permalinks, the datasource access check should be more restrictive to prevent potential security risks.
-
-
superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx - 1
-
UI Flash on Load · Line 64-64The initial ready state is set to true, causing the component to render unloaded content briefly before the useEffect sets it to false and shows the loading spinner. This creates a flash of empty UI on modal open.
Code suggestion
@@ -64,1 +64,1 @@ - const [ready, setReady] = useState(true); + const [ready, setReady] = useState(false);
-
-
superset/embedded_chart/api.py - 1
-
Missing input validation · Line 238-239Without validation, invalid types for allowed_domains (e.g., not a list) or ttl_minutes (e.g., negative or non-integer) could cause runtime errors or unexpected behavior like immediate expiration. Add checks to ensure allowed_domains is a list of strings and ttl_minutes is a positive integer.
Code suggestion
@@ -238,3 +238,7 @@ - allowed_domains: list[str] = body.get("allowed_domains", []) - ttl_minutes: int = body.get("ttl_minutes", 60) - + allowed_domains: list[str] = body.get("allowed_domains", []) + if not isinstance(allowed_domains, list) or not all(isinstance(domain, str) for domain in allowed_domains): + return self.response_400(message="allowed_domains must be a list of strings") + ttl_minutes: int = body.get("ttl_minutes", 60) + if not isinstance(ttl_minutes, int) or ttl_minutes <= 0: + return self.response_400(message="ttl_minutes must be a positive integer") +
-
-
embed-demo.html - 1
-
Missing License Header · Line 1-1This new file requires the Apache license header per project standards for all code files. The header should be added as an HTML comment before the DOCTYPE.
Code suggestion
@@ -0,0 +1,18 @@ +<!-- +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +-->
-
-
superset/models/embedded_chart.py - 1
-
Correctness: Domain parsing logic · Line 58-58The allowed_domains property splits allow_domain_list by comma but does not strip whitespace or filter empty entries, which could lead to incorrect domain matching (e.g., 'domain1, domain2' yields ['domain1', ' domain2']). Consider refining it to handle common input variations for better reliability.
Code suggestion
@@ -58,1 +58,2 @@ - return self.allow_domain_list.split(",") if self.allow_domain_list else [] + domains = self.allow_domain_list.split(",") if self.allow_domain_list else [] + return [domain.strip() for domain in domains if domain.strip()]
-
-
superset-frontend/src/pages/EmbeddedChartsList/index.tsx - 1
-
Inconsistent null handling in toasts · Line 97-97The success and error toast messages in handleDelete use chart.chart_name directly, but the modal description uses chart.chart_name || t('Untitled'). This inconsistency could display 'null' in toasts if chart_name is null, unlike the modal which shows 'Untitled'. Update the toasts to match the modal's pattern for uniform user experience.
Code suggestion
@@ -92,11 +92,11 @@ - try { - await SupersetClient.delete({ - endpoint: `/api/v1/chart/${chart.chart_id}/embedded`, - }); - addSuccessToast(t('Embedding disabled for %s', chart.chart_name)); - fetchEmbeddedCharts(); - } catch (error) { - addDangerToast(t('Error disabling embedding for %s', chart.chart_name)); - } finally { + try { + await SupersetClient.delete({ - endpoint: `/api/v1/chart/${chart.chart_id}/embedded`, - }); - addSuccessToast(t('Embedding disabled for %s', chart.chart_name || t('Untitled'))); - fetchEmbeddedCharts(); - } catch (error) { - addDangerToast(t('Error disabling embedding for %s', chart.chart_name || t('Untitled'))); - } finally {
-
-
superset/daos/chart.py - 1
-
Incorrect type hint in DAO method · Line 128-131The create method in EmbeddedChartDAO has an incorrect type hint for the 'item' parameter—it uses 'EmbeddedChartDAO' instead of 'EmbeddedChart', which is the generic type T for this DAO. The return type 'Any' should also be 'EmbeddedChart' for proper type safety, as required by the project's type hinting standards.
-
-
superset-frontend/src/explore/components/EmbedCodeContent.jsx - 1
-
Date formatting inconsistency · Line 165-177The code uses native Date.toLocaleString() for formatting the expiration date, but the rest of the codebase consistently uses dayjs (via extendedDayjs) for date handling. To maintain consistency, consider switching to extendedDayjs(embedData.expires_at).format('LLL'), which provides localized formatting and aligns with patterns seen in files like QueryTable and LastUpdated components.
Code suggestion
@@ -23,2 +23,3 @@ - import { CopyToClipboard } from 'src/components'; - + import { CopyToClipboard } from 'src/components'; + import { extendedDayjs } from '@superset-ui/core/utils/dates'; + @@ -171,3 +172,3 @@ - {t('Token expires')}:{' '} - {new Date(embedData.expires_at).toLocaleString()} - </Typography.Text> + {t('Token expires')}:{' '} + {extendedDayjs(embedData.expires_at).format('LLL')} + </Typography.Text>
-
-
superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py - 1
-
Code consistency improvement · Line 150-150Other MCP tools consistently use ctx.user for accessing the current user context. While g.user works, switching to ctx.user improves code consistency across the MCP service.
-
-
superset/commands/chart/delete.py - 1
-
Style: Missing docstrings · Line 74-84The new class and methods lack docstrings, required for all new functions and classes. Add brief descriptions explaining their purpose.
-
Review Details
-
Files reviewed - 35 · Commit Range:
929c256..eaa3ca3- embed-demo.html
- superset-frontend/src/dashboard/components/EmbeddedChartModal/index.tsx
- superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx
- superset-frontend/src/dashboard/types.ts
- superset-frontend/src/embeddedChart/index.tsx
- superset-frontend/src/explore/components/EmbedCodeContent.jsx
- superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
- superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.jsx
- superset-frontend/src/pages/EmbeddedChartsList/index.tsx
- superset-frontend/src/views/routes.tsx
- superset-frontend/webpack.config.js
- superset/charts/api.py
- superset/charts/schemas.py
- superset/commands/chart/delete.py
- superset/commands/chart/exceptions.py
- superset/config.py
- superset/daos/chart.py
- superset/embedded_chart/__init__.py
- superset/embedded_chart/api.py
- superset/embedded_chart/exceptions.py
- superset/embedded_chart/view.py
- superset/initialization/__init__.py
- superset/mcp_service/app.py
- superset/mcp_service/embedded_chart/__init__.py
- superset/mcp_service/embedded_chart/schemas.py
- superset/mcp_service/embedded_chart/tool/__init__.py
- superset/mcp_service/embedded_chart/tool/get_embeddable_chart.py
- superset/migrations/versions/2026-01-08_16-45_50aff65919a0_add_embedded_charts_table.py
- superset/models/embedded_chart.py
- superset/models/slice.py
- superset/security/guest_token.py
- superset/security/manager.py
- superset/views/embedded_charts.py
- tests/integration_tests/charts/api_tests.py
- tests/integration_tests/security_tests.py
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
| # Build iframe URL using request host | ||
| base_url = request.host_url.rstrip("/") | ||
| iframe_url = f"{base_url}/embedded/chart/?permalink_key={permalink_key}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The iframe_url uses a query parameter (?permalink_key=), but the GET endpoint expects a path parameter (/<permalink_key>). This mismatch will cause embedded charts to fail loading, as the URL won't match the exposed route.
Code suggestion
Check the AI-generated fix before applying
| # Build iframe URL using request host | |
| base_url = request.host_url.rstrip("/") | |
| iframe_url = f"{base_url}/embedded/chart/?permalink_key={permalink_key}" | |
| # Build iframe URL using request host | |
| base_url = request.host_url.rstrip("/") | |
| iframe_url = f"{base_url}/embedded/chart/{permalink_key}" |
Code Review Run #5db474
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| class EmbeddedChartResponseSchema(Schema): | ||
| uuid = fields.String() | ||
| allowed_domains = fields.List(fields.String()) | ||
| chart_id = fields.Integer() | ||
| changed_on = fields.DateTime() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The uuid field should use fields.UUID() to ensure proper UUID validation during serialization, matching the model's UUIDType column. Similarly, chart_id should use fields.Int() for consistency with other schemas like ChartGetResponseSchema, as fields.Integer() is an alias but not the standard used in this file.
Code suggestion
Check the AI-generated fix before applying
| class EmbeddedChartResponseSchema(Schema): | |
| uuid = fields.String() | |
| allowed_domains = fields.List(fields.String()) | |
| chart_id = fields.Integer() | |
| changed_on = fields.DateTime() | |
| class EmbeddedChartResponseSchema(Schema): | |
| uuid = fields.UUID() | |
| allowed_domains = fields.List(fields.String()) | |
| chart_id = fields.Int() | |
| changed_on = fields.DateTime() |
Code Review Run #5db474
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
…_chart - Validate that timeseries viz_types (echarts_timeseries_*) have a datetime column configured via x_axis, granularity_sqla, or dataset's main_dttm_col - Return helpful error message suggesting to use 'bar' for categorical data - Update schema description to explain categorical vs timeseries chart types - Update docstring with clear examples for both use cases 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
dist_bar is a legacy chart type that may not be registered in newer Superset versions. Use 'bar' instead for categorical bar charts. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Move TIMESERIES_VIZ_TYPES and CATEGORICAL_VIZ_TYPES to chart_utils.py - Add resolve_dataset() shared utility for dataset lookup with access check - Add validate_timeseries_config() shared utility for viz_type validation - Update get_embeddable_chart to use shared utilities - Reduces code duplication with generate_chart tool 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
Code Review Agent Run #ef6e5aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
SUMMARY
Add embeddable charts with guest token authentication, accessible via both:
get_embeddable_chart) - For AI agents to create charts programmaticallyKey Features:
CHART_PERMALINKresource type for secure embeddingCONFIGURATION
Required Feature Flag
Guest Token Settings
GUEST_TOKEN_JWT_SECRET"test-guest-secret-change-me"openssl rand -base64 42GUEST_TOKEN_JWT_EXP_SECONDS300(5 min)GUEST_TOKEN_JWT_ALGO"HS256"GUEST_TOKEN_JWT_AUDIENCENoneGUEST_TOKEN_HEADER_NAME"X-GuestToken"Feature Flags
EMBEDDABLE_CHARTSTrueIframe Embedding (Development)
For local development, you may need to allow iframe embedding:
Example Production Config
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
New "Embed chart" menu option:

Embedded chart rendering:
TESTING INSTRUCTIONS
Testing via UI
EMBEDDED_SUPERSETfeature flag insuperset_config.pyTesting via MCP (for AI Agents)
1. Create embeddable chart:
2. Embed in HTML:
ADDITIONAL INFORMATION
Architecture:
CHART_PERMALINKresource typeImportant Notes:
ttl_minutesparameter controls permalink expirationGUEST_TOKEN_JWT_EXP_SECONDScontrols guest token expiration